-
Notifications
You must be signed in to change notification settings - Fork 462
MCO-1042: ocb-api implementation in MCO #4271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@cdoern: This pull request references MCO-1042 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoern The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| } | ||
| } | ||
|
|
||
| // NewMachineConfigPoolCondition creates a new MachineConfigPool condition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Nice additions!
|
|
||
| // NewMachineConfigPoolCondition creates a new MachineConfigPool condition. | ||
| func NewMachineOSBuildCondition(condType string, status metav1.ConditionStatus, reason, message string) *metav1.Condition { | ||
| return &metav1.Condition{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: Would it make sense to put these helpers in a separate file called, e.g., machineosbuildcondition.go? This would be purely for organizational purposes.
| // equal to what the pool expects. | ||
| func (l *LayeredNodeState) IsUnavailable(mcp *mcfgv1.MachineConfigPool) bool { | ||
| return isNodeUnavailable(l.node) && l.isDesiredImageEqualToPool(mcp) | ||
| func (l *LayeredNodeState) IsUnavailable(mcp *mcfgv1.MachineConfigPool, layered bool) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Is there an advantage to passing in whether a pool is layered or not instead of inferring it from the pool object?
I ask because when I wrote this, I intended for LayeredNodeState to infer whether the pool is layered or not. To be clear: I'm fine with this change, I just want to understand the reason for it 😄
EDIT: I think I understand the reason for this change, please correct me if I'm wrong: We're now inferring whether a pool is layered or not based upon both the presence of the layered label, as well as the presence of both an associated MachineOSBuild and MachineOSConfig object, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should only depend on the existence of the MachineOSConfig object as the opt in mechanism to layering.
I kept the label thing here just for testing
| val, ok := l.node.Annotations[anno] | ||
|
|
||
| if lps.IsLayered() && lps.HasOSImage() { | ||
| if lps.HasOSImage() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: On the surface, it makes sense that if a pool has an OS image that it is layered. But there are two corner-cases that checking for both the presence of the layered label (IsLayered()) and the OS image annotation presence (HasOSImage()) catches:
- When a pool is opted into layering, it won't immediately have an OS image because of the time it takes BuildController to build the image and apply it to the pool. In this situation, the node image annotation will not be equal to the pool, so this check should return false.
- When a pool is opted out of layering, the OS image will be removed from the pool. In this situation, the node will have the image annotation when the pool does not have an OS image, so this check should return false.
| // OS image value, if the value is available. If the pool is not layered, then | ||
| // any image annotations should not be present on the node. | ||
| func (l *LayeredNodeState) isImageAnnotationEqualToPool(anno string, mcp *mcfgv1.MachineConfigPool) bool { | ||
| func (l *LayeredNodeState) isImageAnnotationEqualToPool(anno string, mcp *mcfgv1.MachineConfigPool, layered bool) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quibble: The layering parameter is unused in this function.
| } | ||
|
|
||
| for _, config := range configList { | ||
| if config.Spec.MachineConfigPool.Name == pool.Name { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: I wish this was more ergonomic. I know that listers support label selectors, but then we'd need something to set a label on each MachineOSConfig and MachineOSBuild object. It'd be nice if listers supported field selectors too.
(To be clear: I'm not asking you to add labels.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, I think for GA we should support matching labels on these new objects
| for _, config := range configList { | ||
| if config.Spec.MachineConfigPool.Name == pool.Name { | ||
| ourConfig = config | ||
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: What should happen if ourConfig / ourBuild is nil, meaning that there is no config or build for the given MachineConfigPool? Should we return an error in that case?
EDIT: Oh, I see that you've added the nil check to IsLayeredPool().
| // ready so we can update both the nodes' desired MachineConfig and desired | ||
| // image annotations simultaneously. | ||
|
|
||
| func (ctrl *Controller) GetConfigAndBuild(pool *mcfgv1.MachineConfigPool) (*mcfgv1alpha1.MachineOSConfig, *mcfgv1alpha1.MachineOSBuild, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quibble: If this method is not intended to be used outside of the NodeController, it should be renamed to getConfigAndBuild() because of Go's export rules.
|
|
||
| // If the MachineConfigPool has the build object reference, we just want to | ||
| // update the MachineConfigPool's status. | ||
| if ps.HasBuildObjectRef(objRef) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: This never worked quite the way that I wanted it to only because RenderController would wipe out the object reference to the running build. In the long-run, I'd like to move away from this as the way that we're associating a running build with a MachineConfigPool. Instead, I think adding labels to the MachineConfigPool and the ephemeral build objects that we can use label queries for might be the better option.
(To be clear: I'm not asking you to change anything; I'm commenting on something that I think may have been a mistake on my part.)
| ibr, err := ctrl.prepareForBuild(inputs) | ||
| var ourConfig *mcfgv1alpha1.MachineOSConfig | ||
| for _, c := range machineOSConfigs { | ||
| if c.Spec.MachineConfigPool.Name == mosb.Spec.MachineConfigPool.Name { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: Since it looks like we're repeating this pattern here and in NodeController, lets add a helper in the apihelpers package that does something like:
func GetMachineOSConfigForPool(moscLister MoscListerInterface, pool *mcfgv1.MachineConfigPool) (*mcfgv1alpha1.MachineOSConfig, error) {
machineOSConfigs, err := ctrl.machineOSConfigLister.List(labels.Everything())
if err != nil {
return nil, err
}
for _, c := range machineOSConfigs {
if c.Spec.MachineConfigPool.Name == mosb.Spec.MachineConfigPool.Name {
return c, nil
}
}
// https://github.com/kubernetes/apimachinery/blob/master/pkg/api/errors/errors.go#L144
return nil, apierrors.NewNotFound(...)
}Because we return an error if we can't find the object, the caller can check for that error and determine what to do next. We should also add an equivalent helper for MachineOSBuilds as well. Although for that particular case, we may want to return a list instead.
| } | ||
|
|
||
| ps := newPoolState(pool) | ||
| var mosb *mcfgv1alpha1.MachineOSBuild |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: Let's hoist this into the apihelpers package. See: https://github.com/openshift/machine-config-operator/pull/4271/files#r1532244342.
Also, we may eventually have multiple builds per pool, depending on how we decide to implement multi-arch builds.
| Output: buildv1.BuildOutput{ | ||
| To: &corev1.ObjectReference{ | ||
| Name: i.FinalImage.Pullspec, | ||
| Name: i.MachineOSConfig.Spec.BuildInputs.FinalImagePullspec, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: It is not obvious (and I apologize for that) that this value is mutated from what is retrieved from the on-cluster-build-config ConfigMap. This occurs in the getOnClusterBuildConfig() method. Specifically, the name of the rendered MachineConfig replaces the user-provided tag. The user-provided tag is ignored, although maybe we should update the user-provided tag instead.
(To be clear: I'm just bringing this to your attention.)
| VolumeSource: corev1.VolumeSource{ | ||
| Secret: &corev1.SecretVolumeSource{ | ||
| SecretName: i.FinalImage.PullSecret.Name, | ||
| SecretName: i.MachineOSConfig.Spec.BuildInputs.FinalImagePullspec, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Did you mean i.MachineOSConfig.Spec.BuildInputs.FinalImagePushSecret.Name here?
| ps.DeleteBuildRefForCurrentMachineConfig() | ||
| // Set the annotation or field to point to the newly-built container image. | ||
| klog.V(4).Infof("Setting new image pullspec for %s to %s", ps.Name(), imagePullspec) | ||
| ps.SetImagePullspec(imagePullspec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This value should be provided by the FinalPullspec() method on both the ImageBuilder and CustomPodBuilder objects. The reason is because both of those objects return the digested image pullspec instead of the tagged image pullspec.
e7c8704 to
fabc9b2
Compare
a6eb27c to
6e8eca8
Compare
|
@cdoern: This pull request references MCO-1042 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| } { | ||
| } | ||
|
|
||
| // this is for the FG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might need this @inesqyx otherwise clusters may fail trying to list this type? I am unsure
You'll need to make sure the MOSC and MOSB CRDs are installed correctly upon cluster installation or else this'll cause issues.
Signed-off-by: Charlie Doern <[email protected]>
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@cdoern: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/hold for QE testing |
|
Pre-merge testing has been tracked here: https://issues.redhat.com/browse/MCO-1149 /label qe-approved |
|
@cdoern: This pull request references MCO-1042 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/remove-label qe-approved |
|
@cdoern: This pull request references MCO-1042 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This is the implementation of the
MachineOSConfigandMachineOSBuildAPIs in the MCO.All usages of pool config and status in the build controller to store build information have been removed. Instead we now depend on a per-pool
MachineOSConfigfor user level OCB options and a per-buildMachineOSBuildobject created in its wake.A sample MachineOSConfig looks as follows:
This is the bare minimum spec needed to apply a MachineOSConfig object.